Remote authenticator and authorizer #234
Conversation
moroten
force-pushed
the
remote-auth
branch
3 times, most recently
from
8e43646 to
47d3807
Compare
|
@EdSchouten Welcome back from Christmas holiday. I forgot to bring this up for discussion yesterday. Any comments? No hurry, take your time. |
|
I've now rebased and fixed all the review comments. The changes were done in |
|
@EdSchouten friendly ping. Would you like me to click "Resolve conversation"? |
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| type remoteRequestAuthenticator struct { |
There was a problem hiding this comment.
What are your thoughts on calling this requestHeadersAuthenticator as well? Yes, it would be the same name as the underlying interface, but because it's in a different package (auth.RequestHeaderAuthenticator vs. http.NewRequestHeadersAuthenticator()) they would still be sufficiently distinct.
The reason I'm bringing this up is because there isn't really anything "remote" about this type. It would be perfectly fine to have an implementation of auth.RequestHeadersAuthenticator that looks at stuff in a SQLite database.
This also makes me wonder: should we reimplement our JWT auth to be built on top of this interface as well?
There was a problem hiding this comment.
This also makes me wonder: should we reimplement our JWT auth to be built on top of this interface as well?
Is that just to extract a single header into the JWT handler? Maybe I didn't get how you meant.
There was a problem hiding this comment.
Right now we have these:
-rw-r--r-- 1 ed staff 1240 Dec 15 10:38 pkg/grpc/jwt_authenticator.go
-rw-r--r-- 1 ed staff 1048 Dec 15 10:38 pkg/http/jwt_authenticator.go
But considering that we now have pkg/{grpc,http}/request_header_authenticator.go, we no longer need these, right? We could throw them away and provide a single implementation of auth.RequestHeadersAuthenticator that does the same thing.
There was a problem hiding this comment.
Now, I get it. I've pushed an update which I've verified in bb-deployments docker-compose for the gRPC header handling.
|
|
||
| message RemoteHttpRequestAuthenticationPolicy { | ||
| // The remote authenticator to grant or deny access the HTTP request. | ||
| buildbarn.configuration.grpc.RemoteAuthenticationPolicy backend = 1; |
There was a problem hiding this comment.
Shouldn't this message live in buildbarn.configuration.auth, because it's independent of gRPC/HTTP?
There was a problem hiding this comment.
This creates a circular dependency between grpc.proto and auth.proto: grpc.ServerConfiguration -> grpc.AuthenticationPolicy -> auth.RemoteAuthenticator -> grpc.ClientConfiguration. If we split into grpc_client.proto and grpc_server.proto, it will work. Shall I do that?
| // if a proxy has already performed authentication where the resulting HTTP | ||
| // request's headers need to be verified and processed in a custom way to | ||
| // produce buildbarn.auth.AuthenticationMetadata. | ||
| RemoteGrpcRequestAuthenticationPolicy remote = 8; |
There was a problem hiding this comment.
Would it make sense to replace this field and the jwt with one called request_headers or something, containing a oneof for remote and jwt inside? That way people can add new request header based authenticators without requiring further code changes to HTTP and gRPC.
There was a problem hiding this comment.
Looking at the factory code, it makes sense.
|
@EdSchouten Two open questions at the moment:
Would you like the fixes as fixup-commits or rebased (squashed) into the existing commits? |
|
It would be nice to put Both It starts to become a bit of a refactoring, but I'm up for doing it if you think this is the way to do it. Just a detail, shall the name be |
Ugh! Well, let's not go that far for now. Let's just accept that there's a minimal amount of overlap between the two then. Can you at least add a TODO/comment/... something to the code to document that this overlap exists? |
There was a problem hiding this comment.
Thanks a lot for working on this, @moroten. Really appreciated.
I have some final (small) remarks. Be sure to address those, and I'll merge this swiftly.
pkg/auth/remote_authorizer.go
Outdated
| return response.err | ||
| } | ||
| // No valid cache entry available. Deduplicate requests. | ||
| if responseReady, ok := a.pendingRequests[requestKey]; ok { |
There was a problem hiding this comment.
Tiny nit: for readability, I'd appreciate it that we wrote code like this:
if foo {
A
// no return here
} else {
B
return
}As this instead:
if !foo {
B
return
}
AThis makes it more explicit that the select { case <-responseReady: ... } falls through and causes the loop to run again, as it'll now be closer to the bottom of the loop.
There was a problem hiding this comment.
Also changed
select {
case <-ctx.Done():
return ctx.Err()
case <-wakeupChan:
A
}
into
select {
case <-ctx.Done():
return ctx.Err()
case <-wakeupChan:
}
A
Prepare for remote authentication and authorization to avoid future curcular dependency.
The authenticate and authorize tasks can now be sent remotely over gRPC to an external service. This way, custom authentication and authorization does not require a modified builds of the Buildbarn components. To avoid spamming the remote service with calls for every REv2 request and keep the latency low, the verdicts, both allow and deny, are cached for a duration specified in the response from the remote service.
|
Perfect! Will merge as soon as CI is happy. |
I added a comment in |
The authenticate and authorize tasks can now be sent remotely over gRPC to an external service. This way, custom authentication and authorization does not require a modified builds of the Buildbarn components.
To avoid spamming the remote service with calls for every REv2 request and keep the latency low, the verdicts, both allow and deny, are cached for a duration specified in the response from the remote service.